-
Notifications
You must be signed in to change notification settings - Fork 55
feat: add configurable show desktop feature #1222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideThis PR integrates DConfig into the ShowDesktop applet to introduce an Enable_ShowDesktop configuration option, manages a new visible state in the backend, and propagates it to the QML UI with real-time updates on configuration changes. Sequence diagram for configuration change propagation to UI visibilitysequenceDiagram
participant DConfig
participant ShowDesktop
participant QML_UI
DConfig->>ShowDesktop: valueChanged("Enable_ShowDesktop")
ShowDesktop->>ShowDesktop: onEnableShowDesktopChanged()
ShowDesktop->>ShowDesktop: setVisible(enabled)
ShowDesktop-->>QML_UI: visibleChanged signal
QML_UI->>QML_UI: shouldVisible property updated
Entity relationship diagram for new Enable_ShowDesktop config optionerDiagram
DS_DOCK_CONFIG {
bool Enable_ShowDesktop
}
DS_DOCK_CONFIG ||--o| ShowDesktop : controls visibility
Class diagram for updated ShowDesktop class with DConfig integrationclassDiagram
class ShowDesktop {
+bool visible()
+void setVisible(bool visible)
+void toggleShowDesktop()
+bool checkNeedShowDesktop()
+void onEnableShowDesktopChanged()
-TreelandWindowManager *m_windowManager
-Dtk::Core::DConfig *m_dockConfig
-bool m_visible
<<signal>> visibleChanged
}
ShowDesktop --|> DApplet
ShowDesktop o-- DConfig
ShowDesktop o-- TreelandWindowManager
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @wjyrich - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `panels/dock/showdesktop/showdesktop.cpp:32` </location>
<code_context>
+ // 创建DConfig实例来读取dock配置
+ m_dockConfig = DConfig::create("org.deepin.dde.shell", "org.deepin.ds.dock", QString(), this);
+
+ if (m_dockConfig) {
+ // 监听配置变化
+ connect(m_dockConfig, &DConfig::valueChanged, this, [this](const QString &key) {
+ if (key == "Enable_ShowDesktop") {
+ onEnableShowDesktopChanged();
+ }
+ });
+ }
</code_context>
<issue_to_address>
Consider handling DConfig initialization failure more explicitly.
If m_dockConfig is null, consider logging a warning or handling the failure to improve visibility and debugging of configuration issues.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
m_dockConfig = DConfig::create("org.deepin.dde.shell", "org.deepin.ds.dock", QString(), this);
if (m_dockConfig) {
// 监听配置变化
connect(m_dockConfig, &DConfig::valueChanged, this, [this](const QString &key) {
if (key == "Enable_ShowDesktop") {
onEnableShowDesktopChanged();
}
});
}
=======
m_dockConfig = DConfig::create("org.deepin.dde.shell", "org.deepin.ds.dock", QString(), this);
if (m_dockConfig) {
// 监听配置变化
connect(m_dockConfig, &DConfig::valueChanged, this, [this](const QString &key) {
if (key == "Enable_ShowDesktop") {
onEnableShowDesktopChanged();
}
});
} else {
qWarning() << "Failed to initialize DConfig for ShowDesktop: dock config is null.";
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `panels/dock/showdesktop/showdesktop.cpp:23` </location>
<code_context>
ShowDesktop::ShowDesktop(QObject *parent)
: DApplet(parent)
, m_windowManager(nullptr)
</code_context>
<issue_to_address>
Consider replacing the raw DConfig pointer and separate slot with an in-place DConfig member and a single lambda to update visibility.
Consider ditching the raw `DConfig*` + validity checks + extra slot and just keep a `DConfig` member initialized in-place, read the default once, and hook one lambda to update your `m_visible`. You can collapse ctor + init into:
```cpp
// showdesktop.h
class ShowDesktop : public DApplet {
Q_OBJECT
Q_PROPERTY(bool visible READ visible WRITE setVisible NOTIFY visibleChanged)
public:
explicit ShowDesktop(QObject *parent = nullptr);
bool visible() const { return m_visible; }
void setVisible(bool v) {
if (m_visible == v) return;
m_visible = v;
Q_EMIT visibleChanged();
}
signals:
void visibleChanged();
private:
DConfig m_dockConfig{"org.deepin.dde.shell", "org.deepin.ds.dock"};
bool m_visible{true};
};
```
```cpp
// showdesktop.cpp
ShowDesktop::ShowDesktop(QObject *parent)
: DApplet(parent)
, m_visible(m_dockConfig.value("Enable_ShowDesktop", true).toBool())
{
// One shot: emit initial state
Q_EMIT visibleChanged();
// Update on config change
connect(&m_dockConfig, &DConfig::valueChanged, this, [this](const QString &key) {
if (key == QLatin1String("Enable_ShowDesktop")) {
setVisible(m_dockConfig.value(key, true).toBool());
}
});
DApplet::init();
}
```
This:
- Removes the raw pointer + `.isValid()` guards
- Inlines the “read default” & “watch for changes” in one place
- Eliminates a separate `onEnableShowDesktopChanged()` slot
- Keeps all existing behavior intact but cuts ~20 lines of boilerplate.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // 监听配置变化 | ||
| connect(m_dockConfig, &DConfig::valueChanged, this, [this](const QString &key) { | ||
| if (key == "Enable_ShowDesktop") { | ||
| onEnableShowDesktopChanged(); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| bool ShowDesktop::load() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider handling DConfig initialization failure more explicitly.
If m_dockConfig is null, consider logging a warning or handling the failure to improve visibility and debugging of configuration issues.
| m_dockConfig = DConfig::create("org.deepin.dde.shell", "org.deepin.ds.dock", QString(), this); | |
| if (m_dockConfig) { | |
| // 监听配置变化 | |
| connect(m_dockConfig, &DConfig::valueChanged, this, [this](const QString &key) { | |
| if (key == "Enable_ShowDesktop") { | |
| onEnableShowDesktopChanged(); | |
| } | |
| }); | |
| } | |
| m_dockConfig = DConfig::create("org.deepin.dde.shell", "org.deepin.ds.dock", QString(), this); | |
| if (m_dockConfig) { | |
| // 监听配置变化 | |
| connect(m_dockConfig, &DConfig::valueChanged, this, [this](const QString &key) { | |
| if (key == "Enable_ShowDesktop") { | |
| onEnableShowDesktopChanged(); | |
| } | |
| }); | |
| } else { | |
| qWarning() << "Failed to initialize DConfig for ShowDesktop: dock config is null."; | |
| } |
| ShowDesktop::ShowDesktop(QObject *parent) | ||
| : DApplet(parent) | ||
| , m_windowManager(nullptr) | ||
| , m_dockConfig(nullptr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider replacing the raw DConfig pointer and separate slot with an in-place DConfig member and a single lambda to update visibility.
Consider ditching the raw DConfig* + validity checks + extra slot and just keep a DConfig member initialized in-place, read the default once, and hook one lambda to update your m_visible. You can collapse ctor + init into:
// showdesktop.h
class ShowDesktop : public DApplet {
Q_OBJECT
Q_PROPERTY(bool visible READ visible WRITE setVisible NOTIFY visibleChanged)
public:
explicit ShowDesktop(QObject *parent = nullptr);
bool visible() const { return m_visible; }
void setVisible(bool v) {
if (m_visible == v) return;
m_visible = v;
Q_EMIT visibleChanged();
}
signals:
void visibleChanged();
private:
DConfig m_dockConfig{"org.deepin.dde.shell", "org.deepin.ds.dock"};
bool m_visible{true};
};// showdesktop.cpp
ShowDesktop::ShowDesktop(QObject *parent)
: DApplet(parent)
, m_visible(m_dockConfig.value("Enable_ShowDesktop", true).toBool())
{
// One shot: emit initial state
Q_EMIT visibleChanged();
// Update on config change
connect(&m_dockConfig, &DConfig::valueChanged, this, [this](const QString &key) {
if (key == QLatin1String("Enable_ShowDesktop")) {
setVisible(m_dockConfig.value(key, true).toBool());
}
});
DApplet::init();
}This:
- Removes the raw pointer +
.isValid()guards - Inlines the “read default” & “watch for changes” in one place
- Eliminates a separate
onEnableShowDesktopChanged()slot - Keeps all existing behavior intact but cuts ~20 lines of boilerplate.
BLumia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @18202781743
8ec4760 to
5f31464
Compare
1. Added Enable_ShowDesktop configuration in dconfig to control show desktop area visibility 2. Implemented DConfig integration to read and monitor configuration changes 3. Added visible property and related signals in ShowDesktop class 4. Connected configuration changes to UI visibility updates 5. Added shouldVisible property in QML to reflect configuration state This change allows users to enable/disable the show desktop area through system settings while maintaining the existing functionality when enabled. The implementation uses DConfig for configuration management and properly propagates changes between backend and UI. feat: 添加可配置的显示桌面功能 1. 在dconfig中添加Enable_ShowDesktop配置项用于控制显示桌面区域的可见性 2. 实现DConfig集成以读取和监控配置变更 3. 在ShowDesktop类中添加visible属性和相关信号 4. 将配置变更连接到UI可见性更新 5. 在QML中添加shouldVisible属性以反映配置状态 此更改允许用户通过系统设置启用/禁用显示桌面区域,同时保留启用时的现有功 能。该实现使用DConfig进行配置管理,并正确地在后端和UI之间传播变更。 Pms: BUG-312457
5f31464 to
65c5708
Compare
deepin pr auto review我对这个代码变更进行审查,主要关注语法逻辑、代码质量、性能和安全性: 语法逻辑
代码质量
性能
安全性
改进建议
总体来说,这个代码变更实现了一个有用的功能,允许用户控制是否显示桌面区域,代码质量较高,实现方式合理。 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, wjyrich The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/forcemerge |
|
This pr force merged! (status: behind) |
This change allows users to enable/disable the show desktop area through system settings while maintaining the existing functionality when enabled. The implementation uses DConfig for configuration management and properly propagates changes between backend and UI.
feat: 添加可配置的显示桌面功能
此更改允许用户通过系统设置启用/禁用显示桌面区域,同时保留启用时的现有功
能。该实现使用DConfig进行配置管理,并正确地在后端和UI之间传播变更。
Pms: BUG-312457
Summary by Sourcery
Add a configurable Show Desktop feature by introducing a new DConfig key, wiring it into the ShowDesktop applet, and exposing its visibility state to QML.
New Features:
Enhancements:
Chores: